-
Notifications
You must be signed in to change notification settings - Fork 529
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
docs: explain chproxy arguments #2689
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 Walkthrough📝 WalkthroughWalkthroughThis pull request includes several changes across multiple files. A new line was added to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Thank you for following the naming conventions for pull request titles! 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
apps/engineering/content/docs/architecture/clickhouse-proxy.mdx (4)
7-7
: Fix typo in the introductionThere's a typo in "runnng" which should be "running".
-Our ClickHouse Proxy is a go app runnng on AWS Apprunner. +Our ClickHouse Proxy is a go app running on AWS Apprunner.
44-44
: Improve grammar in Basic Auth descriptionThe sentence structure could be clearer.
-Basic auth was chosen because that's what ClickHouse uses and allows to reuse their SDKs. +Basic auth was chosen because that's what ClickHouse uses and it allows reusing their SDKs.🧰 Tools
🪛 LanguageTool
[grammar] ~44-~44: Did you mean “reusing”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ... that's what ClickHouse uses and allows to reuse their SDKs. In your sdk, you can spec...(ALLOW_TO)
37-47
: Consider adding security recommendations for Basic AuthWhile the Basic Auth implementation is well documented, consider adding a security note recommending the use of strong passwords and secure transmission of credentials.
Basic auth was chosen because that's what ClickHouse uses and allows to reuse their SDKs. In your sdk, you can specify the url as `https://proxyUser:proxyPassword@host:port` and it will just work. + + Note: Ensure you use strong passwords and always transmit credentials over HTTPS to maintain security.🧰 Tools
🪛 LanguageTool
[grammar] ~44-~44: Did you mean “reusing”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ... that's what ClickHouse uses and allows to reuse their SDKs. In your sdk, you can spec...(ALLOW_TO)
62-67
: Consider adding prerequisites sectionWhile the build instructions are clear, it would be helpful to mention the prerequisites (e.g., Go version requirements) for building from source.
+### Prerequisites + +- Go 1.x or higher + ### Running the service You can run the service either by compiling the go binary via:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
apps/chproxy/README.md
(1 hunks)apps/chproxy/src/main.ts
(0 hunks)apps/engineering/content/docs/architecture/clickhouse-proxy.mdx
(2 hunks)apps/engineering/content/docs/architecture/vault.mdx
(1 hunks)apps/engineering/package.json
(1 hunks)apps/www/package.json
(1 hunks)
💤 Files with no reviewable changes (1)
- apps/chproxy/src/main.ts
✅ Files skipped from review due to trivial changes (3)
- apps/chproxy/README.md
- apps/engineering/content/docs/architecture/vault.mdx
- apps/www/package.json
🧰 Additional context used
🪛 LanguageTool
apps/engineering/content/docs/architecture/clickhouse-proxy.mdx
[grammar] ~44-~44: Did you mean “reusing”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ... that's what ClickHouse uses and allows to reuse their SDKs. In your sdk, you can spec...
(ALLOW_TO)
🔇 Additional comments (2)
apps/engineering/package.json (1)
12-16
: Verify fumadocs package compatibility
The updates to fumadocs packages and addition of fumadocs-openapi align with the documentation improvements. However, let's verify the compatibility between these versions.
✅ Verification successful
Let me gather more information about the usage of fumadocs-openapi in the codebase to ensure it's properly integrated.
Based on the gathered information, I can now generate the final response:
✅ Package updates are compatible and properly integrated
The fumadocs package updates are minor version changes (patch updates) with no breaking changes. The new fumadocs-openapi package is correctly integrated and actively used in the clickhouse-proxy documentation for the Property
component to document environment variables.
- The latest release notes show only bug fixes and trivial updates
- fumadocs-openapi is properly imported and utilized in
apps/engineering/content/docs/architecture/clickhouse-proxy.mdx
- The package versions are aligned with the core fumadocs ecosystem (14.5.4)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any breaking changes or compatibility issues in the updated versions
# Check the changelog/release notes for fumadocs packages
gh api graphql -f query='
{
repository(owner: "fuma-nama", name: "fumadocs") {
releases(first: 5, orderBy: {field: CREATED_AT, direction: DESC}) {
nodes {
tagName
description
}
}
}
}
'
# Check if fumadocs-openapi is being used in the codebase
rg -l "fumadocs-openapi"
Length of output: 935
Script:
#!/bin/bash
# Check how fumadocs-openapi is being used in the documentation
cat "apps/engineering/content/docs/architecture/clickhouse-proxy.mdx"
# Check for any other potential fumadocs package imports or configurations
rg -l "from ['\"](fumadocs-|@fumadocs)"
Length of output: 2982
apps/engineering/content/docs/architecture/clickhouse-proxy.mdx (1)
20-23
: LGTM!
Clear and concise introduction to the configuration approach.
Summary by CodeRabbit
Release Notes
Documentation
README.md
for the ClickHouse proxy.Chores
package.json
files for both the engineering and landing projects, including version upgrades and the addition of a new dependency.